The copyright header is missing

Added

Would it be possible to change the field names into something more explicit like that maybe:

top_pkg field names are used.

Nit: you need a space here and same below for the case. Can you please check else where in your code that you follow this rule.
https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#space-around-keywords

Fixed

You need to configure your editor to make sure you insert an empty line at the end of each file (it's also part of the coding rules somewhere)

Can you move this typedef into the env_pkg

Moved.

In the names if we can make it clearer which block is connected, as commented in the env_pkg, that would be better for debug purposes

Updated using the top_pkg names

Is this 64 bits width linked with top_pkg::AxiDataWidth? It's better to point to a pkg value, this should be applied wherever required in this PR

This is ID width to handle when the interconnect extends the ID width to be able to back-route the transactions to the managers. However, good spot, the scoreboard cuts the unnecessary bits before anything is pushed to this queue. Replaced by parameter.

Here, the same applies, could you point at the pkg values please axi_addr_start_t

Fixed

You need to add the begin...end for each if whenever it doesn't fit on a single line as a rule. But as we talk about DV here, I'd prefer to add the begin...end and always jump a line this will facilitate breakpoint debug

Added.

This should be break down into multi lines and I think one foreach is useless:

Fixed.

Is there a reason why DATA_WIDTH 1024 is not supported?

Updated to 1024.

Shouldn't this be 512'h1 (or 1024'h1) for all rdata and wdata types?

Updated with the given define.

Nit: the new nomenclature of AXI uses manager and subordinate.

Yes, we talked about this. Is it better if the naming matches the AXI standard or the current RTL. I know, the new standard uses manager and subordinate, but the RTL uses master and slave.

Good point. I'd say that we should follow the latest nomenclature with manager/subordinate. So it might be that the RTL needs to be changed? But from our concern in this VIP, as its purpose goes beyond this particular DUT eventually, I'd suggest to use the new wording. Do you think there will be some confusing piece of code where the 2 kinds will live together?

Updated names.

Can you please formulate this comment more precise?

This is a legacy comment from a previous version. Thanks for spotting it. Removed.

Why don't you point to the values you have defined in the pkg? And I assume the m_data_width should be 1024?

It does not really matter, because the configuration object is set in the test base. However, it does not hurt. Updating.

Why have we got include guards here? They make me raise my eyebrows: fusesoc is supposed to avoid us needing this...

If you have a process to avoid double definition, I can remove the guards. However, they are pretty useful for larger projects.

Firstly, most of the files in question are included by axi4_vip_pkg.sv. Even without any package manager, something has gone VERY wrong if you end up needing include guards on e.g. axi4_vip_cfg.svh.

But you also shouldn't need include guards on axi4_vip_pkg.sv. As you say, things can go wrong if you don't have a package manager (so you end up having to hack together something with include guards). The solution is to use one. This project uses fusesoc, which should solve the problem nicely.

As such, I strongly believe the correct path is to avoid littering the code with extra cruft.

Okay, removing.

One last include guard left! :-) I think the monitor needs to lose it too.

This still remains.

Thanks for defining the masks like I suggested. Annoyingly, this currently assumes that all the signals are at most 32 bits wide (because 1'b1 << abc gets expanded to the width of abc, so 1'b1 << 32 isn't going to do what you want).

I think this is going to be a problem for the data variables here, because m_cfg.m_data_width in the example is 1024. I think something like this would work, but it's really ugly:

  bit [`AXI4_MAX_DATA_WIDTH-1:0] data_mask = (`AXI4_MAX_DATA_WIDTH'b1 << m_cfg.m_data_width) - 1;

I'm convinced this was a very bad suggestion from me (sorry)! I guess the best thing is something like this:

  bit [`AXI4_MAX_DATA_WIDTH-1:0]   data_one = 1;
  bit [`AXI4_MAX_DATA_WIDTH/8-1:0] strb_one = 1;
  bit [`AXI4_MAX_USER_WIDTH-1:0]   user_one = 1;

  bit [`AXI4_MAX_DATA_WIDTH-1:0]   data_mask = (data_one << m_cfg.m_data_width)     - 1;
  bit [`AXI4_MAX_DATA_WIDTH/8-1:0] strb_mask = (strb_one << (m_cfg.m_data_width/8)) - 1;
  bit [`AXI4_MAX_USER_WIDTH-1:0]   user_mask = (user_one << m_cfg.m_user_width)     - 1;

  axi4_vip_item current_burst;

  forever begin
    ...

The version that you've now got is good because it gets the widths right. Unfortunately, the lines end up enormous:

  // Optimized strobe mask width (DATA_WIDTH / 8)
  bit [`AXI4_MAX_DATA_WIDTH-1:0]     data_mask = (`AXI4_MAX_DATA_WIDTH'(1)   << m_cfg.m_data_width)      - 1;
  bit [(`AXI4_MAX_DATA_WIDTH/8)-1:0] strb_mask = ((`AXI4_MAX_DATA_WIDTH/8)'(1) << (m_cfg.m_data_width/8)) - 1;
  bit [`AXI4_MAX_USER_WIDTH-1:0]     user_mask = (`AXI4_MAX_USER_WIDTH'(1)   << m_cfg.m_user_width)      - 1;

(the middle line is 110 characters).

I'd probably suggest the example I wrote above, where the long lines are 88.

I think you may as well squash this commit into the following one. The dependency is a bit pointless to add until we use it and axi_vip_connections.sv doesn't even exist until the next commit.

What's more, the dependency is in the wrong place. I think you actually need it on top_chip_dv_env, don't you?

The idea here was, you need to add the vip first and then you can integrate it.
Could you please send me a link which file are you referring to? I cannot find top_chip_dv_env.

Since the VIP configuration is done in the test base, top_chip_dv_env.core is not enough.

I'm sorry, but I'm not sure that's quite right (or maybe I don't understand your message).

To make things really explicit:

• lowrisc:mocha_dv:top_chip_sim depends on lowrisc:mocha_dv:top_chip_dv_test
• lowrisc:mocha_dv:top_chip_dv_test depends on lowrisc:mocha_dv:top_chip_dv_env
• lowrisc:mocha_dv:top_chip_dv_env depends on lowrisc:dv:axi4_vip:0.1

I think I've said this elsewhere, but there's really no need for this sort of comment. It's analogous to

  // This is an addition.
  a = b + c;

Clearly a pointless comment! :-)

This is clever, but the lines are very long.

I'd suggest avoiding the repeating the names by using a function like this:

function void top_chip_dv_axi_scoreboard::add_mem_range(string     name,
                                                        bit [63:0] start_addr,
                                                        bit [63:0] addr_size);
  bit [63:0] end_addr = start_addr + addr_size - 1;
  mem_map.push_back('{name, start_addr, end_addr});
endfunction

Then these lines end up looking like this one:

  add_mem_range("sub0_sram", top_pkg::SRAMBase, top_pkg::SRAMLength);

Documentation comments belong by the declaration of the function, not its implementation.

These functions need documentation comments that explain what they do.

I think this probably needs to print the mismatching values (and similarly for the other errors in this function).

Trailing whitespace.